-
Notifications
You must be signed in to change notification settings - Fork 7.6k
libc: minimal: stdin: Add getc() implementation and unit tests #93062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Implement getc() in lib/libc/minimal/source/stdin/stdin_console.c. - Add unit tests for getc(), fgetc(), fgets(), and getchar() in tests/lib/sscanf using a mock stdin hook. - Update headers and CMakeLists for new input support. Resolves basic input API coverage for issue zephyrproject-rtos#66943. Signed-off-by: Seyoung Jeong <seyoungjeong@gmail.com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes, but otherwise pretty good.
@stephanosio might have a better suggestion for location and name of the test suite and maybe the file names of the implementation.
zassert_true(strcmp(buf, "World") == 0, "fgets did not read 'World'"); | ||
} | ||
|
||
ZTEST_SUITE(sscanf, NULL, NULL, NULL, NULL, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sscanf
seems like it isn't a particularly descriptive name for a testsuite. I would maybe consider calling this standard_input
or something.
Additionally, pass in the "before" function to be called automatically by the test suite like so.
ZTEST_SUITE(sscanf, NULL, NULL, NULL, NULL, NULL); | |
ZTEST_SUITE(sscanf, NULL, NULL, before, NULL, NULL); |
The one thing I would consider potentially is also creating a way to back up the previous stdin hook if one does not exist, and to restore it in a "after" callback.
int c = getchar(); | ||
zassert_equal(c, 'H', "getchar() did not return 'H'"); | ||
c = getchar(); | ||
zassert_equal(c, 'e', "getchar() did not return 'e'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a loop and check the entirety of the input?
|
||
static int mock_stdin_hook(void) | ||
{ | ||
if (test_input[input_pos] == '\0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the suggested change, EOF
is returned only after there is no input, which is maybe a bit more accurate.
It also allows you to test how the implementation reacts to processing \0
(which should be possible, afaik, since standard input or any other file can contain 0x00
).
Maybe even put some additional text after the \0
.
if (test_input[input_pos] == '\0') { | |
if (input_pos == ARRAY_SIZE(test_input)) { |
void setup_stdin_hook(void) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a "before" callback provided to ZTEST_SUITE()
and then it does not need to be called manually in each test.
void setup_stdin_hook(void) | |
{ | |
static void before(void * arg) | |
{ | |
ARG_UNUSED(arg); |
int c = fgetc(stdin); | ||
zassert_equal(c, 'H', "fgetc(stdin) did not return 'H'"); | ||
c = fgetc(stdin); | ||
zassert_equal(c, 'e', "fgetc(stdin) did not return 'e'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a loop and check the entirety of the input?
int c = getc(stdin); | ||
zassert_equal(c, 'H', "getc(stdin) did not return 'H'"); | ||
c = getc(stdin); | ||
zassert_equal(c, 'e', "getc(stdin) did not return 'e'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a loop and check the entirety of the input?
return zephyr_fread(ptr, size, nitems, stream); | ||
} | ||
|
||
char *gets(char *s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be moved to a separate file.
int fgetc(FILE *stream) | ||
{ | ||
return zephyr_fgetc(stream); | ||
} | ||
|
||
char *fgets(char *s, int size, FILE *stream) | ||
{ | ||
if (s == NULL || size <= 0) { | ||
return NULL; | ||
} | ||
|
||
int i = 0; | ||
int c; | ||
|
||
while (i < size - 1) { | ||
c = fgetc(stream); | ||
if (c == EOF) { | ||
if (i == 0) { | ||
return NULL; | ||
} | ||
break; | ||
} | ||
s[i++] = (char)c; | ||
if (c == '\n') { | ||
break; | ||
} | ||
} | ||
s[i] = '\0'; | ||
return s; | ||
} | ||
|
||
#undef getc | ||
int getc(FILE *stream) | ||
{ | ||
return zephyr_fgetc(stream); | ||
} | ||
|
||
#undef getchar | ||
int getchar(void) | ||
{ | ||
return zephyr_fgetc(stdin); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be moved to separate files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a -1, but just because someone needs to be That Guy who asks the question:
Why do we want this, exactly? Minimal libc is there for embedded code that wants bare-minimum functionality with no extra dependencies. Does studio really qualify? What's the use case for an app that wants to port code using fgetc() but can't just enable picolibc?
Guess one common use case would be to get input from UART console from boot loader or diagnostics firmware? Can it be done by enabling picolibc? Bear with me - I'm new to Zephyr. |
@seyoungjeong picolibc is the default libc and has a smaller footprint in general than this minimal C library. Minimal libc is "minimal" in the sense of "minimal functionality". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the "Minimal libc" documentation:
The minimal libc implements the minimal subset of the ISO/IEC 9899:2011 standard C library functions required to meet the needs of the Zephyr kernel, as defined by the Coding Guidelines Rule A.4.
None of the functions implemented in this PR are required by the Zephyr kernel and it is not foreseeable that they will be required in the future.
As others have already pointed out, the "minimal libc" is supposed to be minimal and Picolibc is intended to serve as the default and fully featured C library for non-"kernel" Zephyr components and applications (see the Coding Guidelines Rule A.4 for the definition of "kernel").
I am blocking this PR on the grounds that:
- Picolibc already supports the C library functions implemented in this PR.
- Zephyr kernel, as defined by the Rule A.4, does not and should not require these functions.
Closed the PR based on comment from @stephanosio |
Resolves basic input API coverage for issue #66943.